[Storage] Create Session Support for GET Requests#47264
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces opt-in “session-based” authentication for eligible GET blob requests (notably downloads) by adding a pipeline policy that can acquire/cache a short-lived session credential per container and use it to sign subsequent GET requests. It also adds the generated CreateSession REST operation/models and a new recorded test covering session behavior.
Changes:
- Added
StorageSessionPolicy+ container-scoped session cache, and plumbeduse_session=Trueinto the sync pipeline creation path. - Added generated
create_sessionoperation + models/enums to support the service’s session creation API. - Added a recorded test and updated proxy sanitizers for session credentials.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure-storage-blob/azure/storage/blob/_shared/policies.py | Adds sync session auth policy + cache and request signing logic. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_shared/base_client.py | Adds use_session kwarg handling and wires StorageSessionPolicy into the sync pipeline. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_shared/policies_async.py | Introduces an async session policy stub (currently unimplemented). |
| sdk/storage/azure-storage-blob/azure/storage/blob/_generated/operations/_container_operations.py | Adds generated create_session request builder + operation. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_generated/aio/operations/_container_operations.py | Adds generated async create_session operation. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_generated/models/_models_py3.py | Adds generated session models: CreateSessionConfiguration, CreateSessionResponse, SessionCredentials. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_generated/models/_azure_blob_storage_enums.py | Adds generated enum AuthenticationType. |
| sdk/storage/azure-storage-blob/azure/storage/blob/_generated/models/init.py | Re-exports new generated models/enums. |
| sdk/storage/azure-storage-blob/azure/apiview-properties.json | Updates API view surface list for new types/ops. |
| sdk/storage/azure-storage-blob/tests/conftest.py | Adds test-proxy sanitizers for session token/key. |
| sdk/storage/azure-storage-blob/tests/test_container.py | Adds recorded test validating bearer-vs-session auth behavior and per-container session usage. |
| sdk/storage/azure-storage-blob/CHANGELOG.md | Documents the new opt-in session authentication feature. |
jalauzon-msft
left a comment
There was a problem hiding this comment.
Some comments on the sync policy itself, did not look at much else yet
| if http_request.method != "GET": | ||
| return False | ||
| parsed = urlparse(http_request.url) | ||
| segments = [s for s in parsed.path.split("/") if s] |
There was a problem hiding this comment.
This is more of a meta point for URL parsing in the PR as a whole but I'm leaving here as an example. Azurite/emulators use URLs like 127.0.0.1/account/container which would break all the container parsing logic. Not sure if we care about that (probably eventually we will need to)
| StorageSessionPolicy( | ||
| account_name=self.account_name, | ||
| session_client_factory=_session_client_factory, | ||
| use_session=True, |
There was a problem hiding this comment.
This is always passed as True? That probably means we don't need it in the policy and if the policy is present, it does its thing?
There was a problem hiding this comment.
The original design I had in mind was to add this policy blindly after StorageBearerTokenPolicy, and depending on the user passed in we just pass use_session thru. Then I realized there are some validation for us to do and that includes creating the session client factory (not worth creating if use_session is not enabled).
There was a problem hiding this comment.
What I think is the best approach here is to replace the explicit use_session flag/kwarg with the policy's mere presence in the pipeline as the enablement signal—so if the StorageSessionPolicy is wired in, session auth is on—and add a runtime _enabled switch that flips off automatically when the service responds with StorageErrorCode.FeatureNotEnabled, so the SDK stops attempting (and retrying) session auth on accounts that don't support it.
| captured = {} | ||
|
|
||
| def make_capture(label): | ||
| def _hook(response): |
There was a problem hiding this comment.
Why hook inside a hook? You shouldn't need that. You may need to declare captured as nonlocal here though.
There was a problem hiding this comment.
It's a way for the hook to distinguish between requests we're making. For example, an upload should use bearer token and download might use sessions. The label helps distinguish that. We still make the checks through captured[label]. I don't think we have to make it nonlocal if we are mutating captured in place.
| for p in getattr(pipeline, "_impl_policies", []): | ||
| if type(p).__name__ == "StorageSessionPolicy": | ||
| return p | ||
| raise AssertionError("StorageSessionPolicy not found on the pipeline") |
There was a problem hiding this comment.
All these inline functions could be helpers, so they aren't duplicated? Even could be shared across sync and async. With the exception of maybe make_capture since it needs a local... but maybe there is something for that too.
| assert auth.startswith("Session ") | ||
| return auth[len("Session ") :].split(":", 1)[0] | ||
|
|
||
| def find_session_policy(pipeline): |
There was a problem hiding this comment.
This is pretty jank, but I'll allow it for now :D
| ) | ||
|
|
||
| def invalidate(self, container_name: str, session_token: Optional[str] = None) -> None: | ||
| if session_token is None: |
There was a problem hiding this comment.
I don't think we need this if. It could remove a session that another thread put there.
0c5537d
into
Azure:storage/sessions-private
No description provided.